Reject FTP commands with parameters containing CR characters#2453
Reject FTP commands with parameters containing CR characters#2453somecookie wants to merge 1 commit into
Conversation
FTP command syntax treats CR and LF characters as command terminators.
All RFC 959 sightings of CR and LF either refer to the CRLF terminator
or treat both characters the same way. For example, RFC 959 defines an
FTP command parameter as a sequence of chars, where:
<char> ::= any of the 128 ASCII characters except <CR> and <LF>
Squid should (and now does) restrict CR use to the command terminator
sequence, especially since some FTP servers treat CRs as command
delimiters -- if we continue to allow embedded CRs in FTP command
parameters than our FtpClient::writeCommand() will assert when trying to
forward those commands to the FTP server. Moreover, we already use that
CR treatment when _parsing_ FTP responses (see
Ftp::Client::parseControlReply()).
When it comes to command termination, CRs are still optional.
A new ban on CRs in FTP command parameter values means that Squid starts
treating some FTP commands as syntactically invalid, using
EarlyErrorKind::MalformedCommand for the first time since its inception
in 2014 commit eacfca8. For example, `PWD\rQUIT` input is now invalid.
N.B. This change removes "RFC 959 Section 3.1.1.5.2" reference. That
reference was wrong: RFC Section 3.1 is about "data representations"
used for file transfers rather than for command syntax. I probably
missed this problem when the addition of that reference was requested at
http://www.squid-cache.org/mail-archive/squid-dev/201408/0023.html
| // inner_space_char = SP / HT / VT / NP ; space_char without CR and LF | ||
| // space_char = SP / HT / VT / NP / CR / LF; any isspace(3) character in "C" locale | ||
|
|
||
| static const auto InlineSpaceChars = " \f\t\v"; |
There was a problem hiding this comment.
Should we align the names with the definition to make the code easier to read, e.g.,
| Current | Grammar token | Suggested name |
|---|---|---|
| InlineSpace | inner_space_char | InnerSpaceChars |
| FullWhiteSpace | space_char | SpaceChars |
| CommandChars | code_char | CodeChars |
| TailChars | parameter_char | ParameterChars |
| LeadingSpace | BWS | BadWhiteSpace |
There was a problem hiding this comment.
We decided not to go for this to keep the diff minimal which makes it clearer what is changed in the parsing logic
There was a problem hiding this comment.
I also noticed the mismatch. It should be fixed. The only question is when: In this PR or in a dedicated polishing followup.
When sketching this code, I preserved old names and old naming patterns because I did not want to change otherwise unchanged official code lines. Emphasizing what we are changing felt more important to me. It still does. However, if others explicitly request that we polish names in this PR (at the expense of diff clarity), I will not object.
| @@ -663,7 +679,7 @@ Ftp::Server::parseOneRequest() | |||
| (void)tok.skipAll(LeadingSpace); // leading OWS and empty commands | |||
There was a problem hiding this comment.
| (void)tok.skipAll(LeadingSpace); // leading OWS and empty commands | |
| (void)tok.skipAll(LeadingSpace); // leading BWS |
There was a problem hiding this comment.
Same as for variable names above
There was a problem hiding this comment.
I would keep this otherwise unchanged line intact because the old comment is not wrong -- old "OWS and empty commands" description matches LeadingSpace definition in PR code. If others want to polish this comment, here is a better variation (BWS includes "empty commands"):
| (void)tok.skipAll(LeadingSpace); // leading OWS and empty commands | |
| (void)tok.skipAll(LeadingSpace); // leading BWS |
or, if you prefer, just
| (void)tok.skipAll(LeadingSpace); // leading OWS and empty commands | |
| (void)tok.skipAll(LeadingSpace); // BWS |
Again, I would not commit this comment change unless explicitly requested by others.
squid-anubis
added
the
M-failed-other
somecookie
changed the title
rousskov
removed
the
M-failed-other
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
Thank you for testing and polishing this improvement.
rousskov
added
S-waiting-for-more-reviewers
4 participants
FTP command syntax treats CR and LF characters as command terminators.
All RFC 959 sightings of CR and LF either refer to the CRLF terminator
or treat both characters the same way. For example, RFC 959 defines an
FTP command parameter as a sequence of chars, where:
Squid should (and now does) restrict CR use to the command terminator
sequence, especially since some FTP servers treat CRs as command
delimiters -- if we continue to allow embedded CRs in FTP command
parameters than our FtpClient::writeCommand() will assert when trying to
forward those commands to the FTP server. Moreover, we already use that
CR treatment when parsing FTP responses (see
Ftp::Client::parseControlReply()).
When it comes to command termination, CRs are still optional.
A new ban on CRs in FTP command parameter values means that Squid starts
treating some FTP commands as syntactically invalid, using
EarlyErrorKind::MalformedCommand for the first time since its inception
in 2014 commit eacfca8. For example,
PWD\rQUITinput is now invalid.This is a Measurement Factory project.